Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix ts with no trailing zeros #18108

Merged
merged 1 commit into from
Jun 7, 2024

Conversation

chaochn47
Copy link
Member

@chaochn47 chaochn47 commented Jun 2, 2024

Please read https://github.com/etcd-io/etcd/blob/main/CONTRIBUTING.md#contribution-flow.

2024-06-02T01:49:05.536Z should be 2024-06-02T01:49:05.536000Z, so precision won't be dropped and it's easier to use string compare when sorting the timestamp. For example 2024-06-02T01:49:05.536Z should be bigger than 2024-06-02T01:49:05.535123Z but the former appears first when sorting the strings.

The RFC3339Nano format removes trailing zeros from the seconds field and thus may not sort correctly once formatted.

A comma or decimal point followed by one or more zeros represents a fractional second, printed to the given number of decimal places. A comma or decimal point followed by one or more nines represents a fractional second, printed to the given number of decimal places, with trailing zeros removed. For example "15:04:05,000" or "15:04:05.000" formats or parses with millisecond precision.

https://pkg.go.dev/time

package main

import (
	"fmt"
	"time"
)

func main() {
	// Define the time in Go's reference format
	t := time.Date(2024, time.June, 2, 1, 49, 5, 536000000, time.UTC)

	formattedOmittedZerosTime := t.Format("2006-01-02T15:04:05.999999Z0700")

	fmt.Println(formattedOmittedZerosTime) // Outputs: 2024-06-02T01:49:05.536Z

	formattedWithTrailingZerosTime := t.Format("2006-01-02T15:04:05.000000Z0700")
	fmt.Println(formattedWithTrailingZerosTime) // Outputs: 2024-06-02T01:49:05.536000Z
}

xref:

  1. Regression in timestamp resolution #15236
  2. Adjust time resolution to microseconds #15239

@chaochn47 chaochn47 requested a review from jmhbnz June 2, 2024 15:34
@serathius
Copy link
Member

Please add a unit test.

@chaochn47 chaochn47 force-pushed the fix-ts-with-no-trailing-zeros branch 2 times, most recently from edbf15a to 1a99fc9 Compare June 6, 2024 23:58
@chaochn47
Copy link
Member Author

chaochn47 commented Jun 7, 2024

Please add a unit test.

Done.

Before

dev-dsk-chaochn-2c-a26acd76 % stress ./logutil.test -test.run=TestEncodeTimePrecisionToMicroSeconds
/tmp/go-stress-20240607T001548-3505485664
--- FAIL: TestEncodeTimePrecisionToMicroSeconds (0.00s)
    zap_test.go:57:
        	Error Trace:	/home/chaochn/workplace/EKS-etcd/src/EKS-etcd/client/pkg/logutil/zap_test.go:57
        	Error:      	Not equal:
        	            	expected: 6
        	            	actual  : 5
        	Test:       	TestEncodeTimePrecisionToMicroSeconds
        	Messages:   	unexpected timestamp 2024-06-07T00:15:49.48821Z
FAIL


ERROR: exit status 1


/tmp/go-stress-20240607T001548-935680383
--- FAIL: TestEncodeTimePrecisionToMicroSeconds (0.00s)
    zap_test.go:57:
        	Error Trace:	/home/chaochn/workplace/EKS-etcd/src/EKS-etcd/client/pkg/logutil/zap_test.go:57
        	Error:      	Not equal:
        	            	expected: 6
        	            	actual  : 4
        	Test:       	TestEncodeTimePrecisionToMicroSeconds
        	Messages:   	unexpected timestamp 2024-06-07T00:15:49.4881Z
FAIL


ERROR: exit status 1


/tmp/go-stress-20240607T001548-48431470
--- FAIL: TestEncodeTimePrecisionToMicroSeconds (0.00s)
    zap_test.go:57:
        	Error Trace:	/home/chaochn/workplace/EKS-etcd/src/EKS-etcd/client/pkg/logutil/zap_test.go:57
        	Error:      	Not equal:
        	            	expected: 6
        	            	actual  : 5
        	Test:       	TestEncodeTimePrecisionToMicroSeconds
        	Messages:   	unexpected timestamp 2024-06-07T00:15:49.49074Z
FAIL


ERROR: exit status 1

After

dev-dsk-chaochn-2c-a26acd76 % stress ./logutil.test -test.run=TestEncodeTimePrecisionToMicroSeconds
5s: 13273 runs so far, 0 failures
10s: 27055 runs so far, 0 failures
15s: 40982 runs so far, 0 failures
20s: 54810 runs so far, 0 failures
25s: 68705 runs so far, 0 failures
30s: 82586 runs so far, 0 failures
35s: 96406 runs so far, 0 failures
40s: 110097 runs so far, 0 failures
45s: 123984 runs so far, 0 failures
50s: 137534 runs so far, 0 failures
55s: 151337 runs so far, 0 failures
1m0s: 165105 runs so far, 0 failures

@chaochn47 chaochn47 force-pushed the fix-ts-with-no-trailing-zeros branch from 1a99fc9 to a31123f Compare June 7, 2024 00:18
@chaochn47 chaochn47 force-pushed the fix-ts-with-no-trailing-zeros branch from a31123f to 7211d9f Compare June 7, 2024 00:20
Copy link
Member

@jmhbnz jmhbnz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM - Retaining precision for ease of formatting/sorting/viewing makes sense. Thanks for adding a unit test @chaochn47

fractionSecondsPrecision = 6 // MicroSeconds
)

func TestEncodeTimePrecisionToMicroSeconds(t *testing.T) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On second thought, it might be a better idea to expand e2e logging tests in ./tests/e2e/logging_test.go.
Good enough for know, we can merge and follow up.

@serathius serathius merged commit 75ad7a8 into etcd-io:main Jun 7, 2024
47 checks passed
@ivanvc ivanvc mentioned this pull request Jun 28, 2024
4 tasks
@jmhbnz
Copy link
Member

jmhbnz commented Jul 6, 2024

/cherrypick release-3.5

@k8s-infra-cherrypick-robot

@jmhbnz: new pull request created: #18288

In response to this:

/cherrypick release-3.5

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

@chaochn47 chaochn47 deleted the fix-ts-with-no-trailing-zeros branch October 14, 2024 18:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

4 participants